Conversation
emyller
requested review from
gagantrivedi and
kyle-ssg
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
bot
added
front-end
Docker builds report
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 10
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6722 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 1311 1313 +2
Lines 48474 48560 +86
=======================================
+ Hits 47617 47703 +86
Misses 857 857 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
I have a non-blocking question and the e2e are failing so it's gonna stale the approval anyways, i'll run them to see what's going on
✖ Segment-part-3
1) AssertionError: waitForElementVisible([data-test="user-feature-switch-1-off"]): expected false to be truthy
Browser: Firefox 143.0 / Debian 13.3
48 |
49 |export const waitForElementVisible = async (selector: string) => {
50 | logUsingLastSection(`Waiting element visible ${selector}`)
51 | return t
52 | .expect(Selector(selector).visible)
> 53 | .ok(`waitForElementVisible(${selector})`, { timeout: LONG_TIMEOUT })
54 |}
55 |
56 |export const waitForElementNotClickable = async (selector: string) => {
57 | logUsingLastSection(`Waiting element visible ${selector}`)
58 | await t
at <anonymous> (/srv/flagsmith/e2e/helpers.cafe.ts:53:6)
at step (/srv/flagsmith/e2e/helpers.cafe.ts:33:23)
at Object.next (/srv/flagsmith/e2e/helpers.cafe.ts:14:53)
at <anonymous> (/srv/flagsmith/e2e/helpers.cafe.ts:8:71)
at __awaiter (/srv/flagsmith/e2e/helpers.cafe.ts:4:12)
at waitForElementVisible (/srv/flagsmith/e2e/helpers.cafe.ts:49:61)
at <anonymous> (/srv/flagsmith/e2e/tests/segment-test.ts:261:30)
at step (/srv/flagsmith/e2e/tests/segment-test.ts:33:23)
at Object.next (/srv/flagsmith/e2e/tests/segment-test.ts:14:53)
at fulfilled (/srv/flagsmith/e2e/tests/segment-test.ts:5:58)
api/features/views.py
Outdated
| override_ordering.append("-has_segment_override") | ||
| if identity_id := query_data.get("identity"): | ||
| queryset = queryset.annotate( | ||
| has_identity_override=Exists( |
There was a problem hiding this comment.
So here, this would make a subquery for each feature row right? I'm comparing to the segment query just above that have a unique_constraint on (feature, segment_id, environment_id), there might be a slight performance difference?
I personally think it's negligeable given that feature limit is 400 per project (~max overrides we can expect) but do you have an idea of the perf loss, how we could measaure it and at what point it could become problematic?
There was a problem hiding this comment.
TL;DR: we should be safe.
So here, this would make a subquery for each feature row right?
Yes, this renders to a subquery in SQL, and translates to a hashed subplan — which roughly means that it executes the subquery only once rather than against each row.
I'm comparing to the segment query just above that have a
unique_constrainton (feature, segment_id, environment_id), there might be a slight performance difference?
In theory, as we're crossing tuples using foreign keys as anchors, the RDBMS should make the best use of each B-tree index associated to them.
I personally think it's negligeable given that feature limit is 400 per project (~max overrides we can expect) but do you have an idea of the perf loss, how we could measaure it and at what point it could become problematic?
The added effort of these conditional filters should cause minimal / negligible performance impact overall. I expect neither data frame size nor query limit to influence performance as we scale.
Here's an example output of explain analyze that illustrates an hypothetical query plan containing both subqueries.
Sort (cost=98.11..98.14 rows=12 width=30) (actual time=0.753..0.764 rows=212 loops=1)
Sort Key: ((hashed SubPlan 2)) DESC, ((hashed SubPlan 4)) DESC, f.name
Sort Method: quicksort Memory: 38kB
Buffers: shared hit=177
-> Index Scan using features_feature_project_id_72859830 on features_feature f (cost=0.42..97.89 rows=12 width=30) (actual time=0.119..0.364 rows=212 loops=1)
Index Cond: (project_id = <project_id>)
Buffers: shared hit=169
SubPlan 2
-> Index Scan using features_featuresegment_segment_id_64602469 on features_featuresegment fs (cost=0.29..3.63 rows=1 width=4) (actual time=0.022..0.026 rows=2 loops=1)
Index Cond: (segment_id = <segment_id>)
Filter: (environment_id = <environment_id>)
Buffers: shared hit=4
SubPlan 4
-> Index Scan using features_featurestate_identity_id_8229f26c on features_featurestate fst (cost=0.42..8.41 rows=8 width=4) (actual time=0.035..0.035 rows=0 loops=1)
Index Cond: (identity_id = <identity_id>)
Buffers: shared hit=3
Planning Time: 1.122 ms
Execution Time: 0.845 ms
|
Looks like legit as persistent and reproducible:
|
api/features/serializers.py
Outdated
| required=False, | ||
| help_text="Integer ID of the segment to retrieve segment overrides for.", | ||
| ) | ||
| identity = serializers.IntegerField( |
There was a problem hiding this comment.
Yes ok, that's working when identities are stored in postgres, but when using edge identities, it passes an UUID.
I'll post a full comment with my findings as we'll need to have 2 implementations
There was a problem hiding this comment.
Can you confirm you were having integer identity.id in the screenshot you shared ?
There was a problem hiding this comment.
Confirmed. It was in my local environment.
Zaimwa9
left a comment
Zaimwa9
left a comment
There was a problem hiding this comment.
So my finding after tracking the E2E failure is that we need 2 distinct approaches depending on using edge identities or storing them in postgres.
The current implementation is all good for postgres, even though the frontend should not pass the identity if using edge (we have an utils that is already used to call the edge endpoints).
For edge:
It's actually simpler, as the information already exists. It's coming from
/api/v1/environments/{env_key}/edge-identities/{identity_as_uuid}/edge-featurestates/all/
and the response is as an array as follows:
[
{
"feature": {
"id": 174685,
"name": "my_go_flag",
"type": "STANDARD"
},
"enabled": true,
"feature_state_value": null,
"overridden_by": null,
"segment": null,
"multivariate_feature_state_values": []
},
{
"feature": {
"id": 174687,
"name": "welcome_page",
"type": "MULTIVARIATE"
},
"enabled": false,
"feature_state_value": null,
"overridden_by": null,
"segment": null,
"multivariate_feature_state_values": [
{
"multivariate_feature_option": {
"value": "plop"
},
"percentage_allocation": 0
}
]
}
]
We already have "overridden_by": null, that can be IDENTITY | SEGMENT. So we could even do it from the frontend (i'd be more at peace to add a sort query param and have it done in the backend as it is the case with postgres identities). Don't hesitate to confirm my finding and I let you chose the best path to take
| search, | ||
| sort, | ||
| getServerFilter(filter), | ||
| { ...getServerFilter(filter), identity: id }, |
There was a problem hiding this comment.
Here we will need to use Utils.getIsEdge() to do something like ...(!isEdge ? { identity: id } : {}),, also for the next useEffect
There was a problem hiding this comment.
Turns out the backend will make use of it either way. See ec88a82.
api/features/serializers.py
Outdated
| required=False, | ||
| help_text="Integer ID of the segment to retrieve segment overrides for.", | ||
| ) | ||
| identity = serializers.IntegerField( |
There was a problem hiding this comment.
Can you confirm you were having integer identity.id in the screenshot you shared ?
emyller
force-pushed
the
fix/sort-features-by-overrides
branch
from
f135c7f to
bb17797
Compare
I really appreciate you digging that up! It has really helped. Brilliant review.
This has connected some dots in my Let me know what you think of this new approach. |
emyller
force-pushed
the
fix/sort-features-by-overrides
branch
from
bb17797 to
7da2818
Compare
emyller
force-pushed
the
fix/sort-features-by-overrides
branch
from
51f0abb to
2c612c9
Compare
2 participants
Closes #6188.
Prioritise displaying overriden features in the dashboard.
For segment overrides:
For identity overrides:
Review effort: 2/5